Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request introduces comprehensive documentation infrastructure for the fgmetric project, including MkDocs configuration, Read the Docs setup, GitHub Actions workflow for PR previews, API documentation pages, user guides, and documentation-related dependencies and build tasks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
.github/workflows/docs.yml (1)
19-19: Pin third-party action to a commit SHA.Line 19 uses a mutable tag (
@v1), which creates a supply-chain risk. GitHub's security guidance recommends pinning to a full commit SHA for reproducibility and safety.💡 Suggested fix
- - uses: readthedocs/actions/preview@v1 + - uses: readthedocs/actions/preview@<full_commit_sha>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docs.yml at line 19, Replace the mutable action reference "readthedocs/actions/preview@v1" with a pinned commit SHA for that action (e.g., "readthedocs/actions/preview@<full-commit-sha>"); update the workflow step that uses readthedocs/actions/preview so it references the specific full commit SHA to ensure immutability and supply-chain safety, and verify the SHA matches the upstream repo's commit for the desired v1 release before committing..readthedocs.yaml (1)
10-11: Pinuvto a specific version in.readthedocs.yaml.Lines 10–11 use
asdf install/global uv latest, which causes non-deterministic builds. Commit a.tool-versionsfile to your repo with a pinned version (e.g.,uv 0.4.4), then update the YAML to install withoutlatestso it reads the pinned version:💡 Suggested fix
- - asdf install uv latest - - asdf global uv latest + - asdf install uv + - asdf global uv 0.4.4Alternatively, commit
.tool-versionsto your repo withuv 0.4.4and useasdf install uv(omit version in YAML).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.readthedocs.yaml around lines 10 - 11, The YAML uses non-deterministic "asdf install uv latest" / "asdf global uv latest"; pin the uv tool instead by adding a .tool-versions file to the repo containing the uv version (e.g., "uv 0.4.4") and update the .readthedocs.yaml lines that call asdf to remove "latest" (either call "asdf install uv" so it reads .tool-versions, or explicitly use the pinned version like "asdf install uv 0.4.4" and "asdf global uv 0.4.4").docs/guide.md (2)
103-109: Document the single-character constraint forcollection_delimiter.Lines [103]-[109] show customization but omit that
collection_delimitermust be exactly one character (enforced infgmetric/collections/_delimited_list.py, Lines 90-95).Proposed change
-The list delimiter defaults to `,` but can be customized per-metric with the `collection_delimiter` class variable: +The list delimiter defaults to `,` but can be customized per-metric with the `collection_delimiter` class variable (must be a single character):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guide.md` around lines 103 - 109, Update the docs to state that the class variable collection_delimiter must be exactly one character (single-character string) — this constraint is enforced by the DelimitedList validator in fgmetric.collections._delimited_list.py (the code that validates collection_delimiter and raises on invalid values). Mention that the default is ",", that you can override it per-metric by setting collection_delimiter = ";" in the Metric subclass, and note that multi-character delimiters will be rejected by the validator.
113-132: Call outCountermodeling constraints explicitly.This section is solid, but it should mention current constraints: one
Counter[...]field per model, non-optional Counter, andCounter[T]whereTisStrEnum(fgmetric/collections/_counter_pivot_table.py, Lines 30-87).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guide.md` around lines 113 - 132, Update the docs to explicitly state the Counter modeling constraints: a model may have at most one Counter[...] field (one Counter field per Metric), the Counter field must be non-optional (not Optional[Counter[...]]) and the Counter generic type parameter must be a StrEnum (i.e., use Counter[StrEnum] like Counter[Base]); reference the implementation enforcement in the fgmetric.collections._counter_pivot_table module and mention the symbols Counter, Metric, StrEnum so readers know where the behavior is enforced and what signatures are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/docs.yml:
- Around line 5-6: The workflow only triggers on new PRs because the
pull_request trigger's types list contains only "opened"; update the
pull_request trigger (the types: setting) to include "synchronize" alongside
"opened" so subsequent commits to the PR retrigger the docs preview workflow and
keep preview data fresh.
In `@docs/guide.md`:
- Around line 67-77: The example uses AlignmentMetric and Path but only imports
MetricWriter, so make the snippet self-contained by either (A) adding the
missing import "from pathlib import Path", importing or defining AlignmentMetric
(e.g., declare class AlignmentMetric(Metric) with fields read_name,
mapping_quality, is_duplicate=False and import Metric from fgmetric) before
using MetricWriter, or (B) if AlignmentMetric is defined elsewhere, add an
explicit "from <module> import AlignmentMetric" import; ensure the metric class
name AlignmentMetric and the Path import are present so
MetricWriter(AlignmentMetric, Path("output.tsv")) and writer.writeall(metrics)
work when copy/pasted.
In `@mkdocs.yml`:
- Line 2: Replace the hardcoded site_url value with the Read the Docs canonical
URL environment variable: update the mkdocs configuration so that the site_url
key uses the READTHEDOCS_CANONICAL_URL environment variable (e.g., set site_url
to the environment variable reference) instead of the fixed
"https://fgmetric.readthedocs.io/en/stable/" so MkDocs emits correct canonical
links for versioned and preview builds.
---
Nitpick comments:
In @.github/workflows/docs.yml:
- Line 19: Replace the mutable action reference "readthedocs/actions/preview@v1"
with a pinned commit SHA for that action (e.g.,
"readthedocs/actions/preview@<full-commit-sha>"); update the workflow step that
uses readthedocs/actions/preview so it references the specific full commit SHA
to ensure immutability and supply-chain safety, and verify the SHA matches the
upstream repo's commit for the desired v1 release before committing.
In @.readthedocs.yaml:
- Around line 10-11: The YAML uses non-deterministic "asdf install uv latest" /
"asdf global uv latest"; pin the uv tool instead by adding a .tool-versions file
to the repo containing the uv version (e.g., "uv 0.4.4") and update the
.readthedocs.yaml lines that call asdf to remove "latest" (either call "asdf
install uv" so it reads .tool-versions, or explicitly use the pinned version
like "asdf install uv 0.4.4" and "asdf global uv 0.4.4").
In `@docs/guide.md`:
- Around line 103-109: Update the docs to state that the class variable
collection_delimiter must be exactly one character (single-character string) —
this constraint is enforced by the DelimitedList validator in
fgmetric.collections._delimited_list.py (the code that validates
collection_delimiter and raises on invalid values). Mention that the default is
",", that you can override it per-metric by setting collection_delimiter = ";"
in the Metric subclass, and note that multi-character delimiters will be
rejected by the validator.
- Around line 113-132: Update the docs to explicitly state the Counter modeling
constraints: a model may have at most one Counter[...] field (one Counter field
per Metric), the Counter field must be non-optional (not Optional[Counter[...]])
and the Counter generic type parameter must be a StrEnum (i.e., use
Counter[StrEnum] like Counter[Base]); reference the implementation enforcement
in the fgmetric.collections._counter_pivot_table module and mention the symbols
Counter, Metric, StrEnum so readers know where the behavior is enforced and what
signatures are required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb5ddc56-3b3f-474a-99e1-9018391de1a5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/workflows/docs.yml.readthedocs.yamlREADME.mddocs/api/metric.mddocs/api/metric_writer.mddocs/contributing.mddocs/guide.mddocs/index.mdmkdocs.ymlpyproject.toml
| types: | ||
| - opened |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For GitHub Actions pull_request_target workflows, which event types are needed so jobs rerun when new commits are pushed to an existing PR?
💡 Result:
To have a pull_request_target workflow re-run when new commits are pushed to an existing PR, you need the synchronize activity type (that’s the PR action GitHub sends on new commits). (docs.github.com)
If you’re explicitly setting types:, include at least:
on:
pull_request_target:
types: [opened, synchronize, reopened](If you omit types, GitHub’s default for pull_request_target already includes opened, synchronize, and reopened.) (docs.github.com)
Citations:
- 1: https://docs.github.com/pt/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows
- 2: https://docs.github.com/pt/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows
Preview link won't refresh after new PR commits.
Line 6 listens only to opened, so later synchronize updates won't retrigger this workflow and preview data can go stale.
💡 Suggested fix
on:
pull_request_target:
types:
- opened
+ - synchronize
+ - reopened🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/docs.yml around lines 5 - 6, The workflow only triggers on
new PRs because the pull_request trigger's types list contains only "opened";
update the pull_request trigger (the types: setting) to include "synchronize"
alongside "opened" so subsequent commits to the PR retrigger the docs preview
workflow and keep preview data fresh.
| ```python | ||
| from fgmetric import MetricWriter | ||
|
|
||
| metrics = [ | ||
| AlignmentMetric(read_name="read1", mapping_quality=60), | ||
| AlignmentMetric(read_name="read2", mapping_quality=30, is_duplicate=True), | ||
| ] | ||
|
|
||
| with MetricWriter(AlignmentMetric, Path("output.tsv")) as writer: | ||
| writer.writeall(metrics) | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && cat -n docs/guide.md | sed -n '67,77p'Repository: fg-labs/fgmetric
Length of output: 436
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && rg "class AlignmentMetric" --type pythonRepository: fg-labs/fgmetric
Length of output: 90
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && rg "class AlignmentMetric"Repository: fg-labs/fgmetric
Length of output: 313
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && cat -n fgmetric/__init__.pyRepository: fg-labs/fgmetric
Length of output: 240
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && cat -n docs/guide.md | sed -n '1,67p' | tail -20Repository: fg-labs/fgmetric
Length of output: 614
🏁 Script executed:
cd "$(git rev-parse --show-toplevel)" && rg "AlignmentMetric" docs/guide.md -B 5 | head -50Repository: fg-labs/fgmetric
Length of output: 949
Make the writing example self-contained.
Lines 75-76 use AlignmentMetric and Path, but the snippet only imports MetricWriter. Copy/pasting this block alone will fail.
Proposed change
```python
+from pathlib import Path
+from fgmetric import Metric
+
+
+class AlignmentMetric(Metric):
+ read_name: str
+ mapping_quality: int
+ is_duplicate: bool = False
+
+
from fgmetric import MetricWriter
metrics = [Alternatively, if the intent is to keep the class definition separate, import it from the earlier section (though this assumes readers have already executed that code block):
```python
+from pathlib import Path
+from fgmetric import Metric
+
+class AlignmentMetric(Metric):
+ read_name: str
+ mapping_quality: int
+ is_duplicate: bool = False
+
from fgmetric import MetricWriter
metrics = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/guide.md` around lines 67 - 77, The example uses AlignmentMetric and
Path but only imports MetricWriter, so make the snippet self-contained by either
(A) adding the missing import "from pathlib import Path", importing or defining
AlignmentMetric (e.g., declare class AlignmentMetric(Metric) with fields
read_name, mapping_quality, is_duplicate=False and import Metric from fgmetric)
before using MetricWriter, or (B) if AlignmentMetric is defined elsewhere, add
an explicit "from <module> import AlignmentMetric" import; ensure the metric
class name AlignmentMetric and the Path import are present so
MetricWriter(AlignmentMetric, Path("output.tsv")) and writer.writeall(metrics)
work when copy/pasted.
| @@ -0,0 +1,63 @@ | |||
| site_name: fgmetric | |||
| site_url: https://fgmetric.readthedocs.io/en/stable/ | |||
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For MkDocs projects hosted on Read the Docs, what is the recommended site_url configuration for versioned builds and canonical URLs?
💡 Result:
For MkDocs projects on Read the Docs (including versioned builds), configure site_url to use Read the Docs’ per-build canonical URL:
# mkdocs.yml
site_url: !ENV READTHEDOCS_CANONICAL_URLRead the Docs computes and exposes READTHEDOCS_CANONICAL_URL (taking into account your default version like latest/stable and any canonical custom domain) and recommends using it for MkDocs’ site_url, which MkDocs uses to emit canonical <link rel="canonical"> URLs. [1] [2] [3]
Use Read the Docs' canonical URL environment variable instead of hardcoding the stable URL.
Hardcoding site_url to the stable URL produces incorrect canonical links in versioned and preview builds. Read the Docs automatically provides READTHEDOCS_CANONICAL_URL (accounting for the default version and any custom domains), which should be used for site_url so MkDocs emits the correct <link rel="canonical"> tags.
Proposed change
-site_url: https://fgmetric.readthedocs.io/en/stable/
+site_url: !ENV [READTHEDOCS_CANONICAL_URL, "https://fgmetric.readthedocs.io/en/stable/"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mkdocs.yml` at line 2, Replace the hardcoded site_url value with the Read the
Docs canonical URL environment variable: update the mkdocs configuration so that
the site_url key uses the READTHEDOCS_CANONICAL_URL environment variable (e.g.,
set site_url to the environment variable reference) instead of the fixed
"https://fgmetric.readthedocs.io/en/stable/" so MkDocs emits correct canonical
links for versioned and preview builds.
2 participants
Summary by CodeRabbit
Release Notes
Documentation
Chores